-
-
Notifications
You must be signed in to change notification settings - Fork 207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat/bridge status controller #5317
Conversation
095dd8d
to
618d362
Compare
027eb7a
to
ccabffd
Compare
b36e602
to
b10c63e
Compare
…g when no srcTxHash
b687a31
to
7cf0f59
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had several comments on dependencies, exports, etc.
Would you mind re-running yarn update-readme-content
? It looks like it might be missing updates to the dependency graph.
}; | ||
|
||
// Actions | ||
type BridgeStatusControllerAction< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also export this type? Otherwise if another controller wanted to just be able to call, say BridgeStatusController:startPollingForBridgeTxStatus
, then it wouldn't be possible to set up the messenger.
Alternatively, what are your thoughts on explicitly defining the action types BridgeStatusControllerStartPollingForBridgeTxStatusAction
and BridgeStatusControllerWipeBridgeStatusAction
? This would be consistent with how action/event types work in other controllers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 33dab2e, added BridgeStatusControllerResetStateAction
as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mcmir Thank you, please let me know if there is anything you need to add or add, thank you
Co-authored-by: Elliot Winkler <[email protected]>
Co-authored-by: Elliot Winkler <[email protected]>
…ore into feat/bridge-status-controller
Done in 0b09fa9 |
…actions and events
}, | ||
"peerDependencies": { | ||
"@metamask/accounts-controller": "^24.0.0", | ||
"@metamask/bridge-controller": "^0.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For future reviewers: there seems to be a bug in the Yarn constraints which forces any controller packages added as dependencies to also be added as peer dependencies. We should fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked the merged file, the added features and changes cover and solve the problems in Metamask, especially the solution to the bridge problem, which was necessary, is available in the packages and will fix the bugs, thank you for the help and cooperation of all of you friends, who with your presence contribute to the development and progress of the community.
Explanation
This PR adds a new controller:
BridgeStatusController
.This controller handles the bridge transaction status fetching and polling from the Bridge API.
References
This is a port of the
BridgeStatusController
from Extension: https://github.com/MetaMask/metamask-extension/tree/main/app/scripts/controllers/bridge-statusSome minor changes were needed to fill in the missing functions and variables from Extension.
This package will be consumed initially by the Metamask Mobile application first. Eventually, we wish to migrate the Extension to use this
core/bridge-status-controller
package.Very closely related to the
BridgeController
: #5276Changelog
@metamask/bridge-status-controller
BridgeStatusController
!@metamask/bridge-controller
BridgeController
FeeType
enum now exported as an enum, not just a typeChecklist